Added calculator app#1110
Open
asthanit1205 wants to merge 1 commit intoflorinpop17:masterfrom
Open
Conversation
📝 WalkthroughWalkthroughA new calculator web application is introduced with three files: HTML providing the user interface with a display and button grid, JavaScript implementing calculator operations (number input, arithmetic operators, clear functions, and expression evaluation), and CSS defining layout styling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
calculator-app/style.css (1)
6-15: Use an explicit grid for deterministic button layout.Right now row wrapping is implicit. Defining
.buttonsas a grid will prevent layout drift across font/zoom/container tweaks.♻️ Proposed CSS update
.calculator { width: 200px; margin: auto; } +.buttons { + display: grid; + grid-template-columns: repeat(4, 40px); + gap: 4px; + justify-content: center; +} + button { width: 40px; height: 40px; - margin: 2px; + margin: 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@calculator-app/style.css` around lines 6 - 15, The button layout should use an explicit CSS grid: add a .buttons rule (targeting the container that holds the button elements inside .calculator) and set display: grid, a fixed grid-template-columns (e.g., repeat(4, 1fr) for four columns) and a gap for spacing so buttons no longer rely on inline wrapping; keep existing button size rules but remove reliance on implicit wrapping so classes .calculator, .buttons and button are used to enforce deterministic layout across zoom/font changes.calculator-app/index.html (1)
13-33: Move inlineonclickhandlers to JS event listeners.All buttons at lines 13-33 use inline handlers. Moving them to JavaScript event listeners improves maintainability and supports stricter Content Security Policy configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@calculator-app/index.html` around lines 13 - 33, Replace inline onclick attributes on all buttons by adding data-action or distinct IDs and wiring them up in your JS: query-select the buttons (e.g., buttons with data-action="digit", data-action="operator", data-action="clear-all", data-action="clear-last", data-action="equals") and attach event listeners that call the existing functions clearAll, clearLast, press (pass the button value), and calculate respectively; remove the onclick attributes from the HTML so behavior is driven by addEventListener calls and use event.currentTarget or dataset to get the button value when invoking press.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@calculator-app/index.html`:
- Line 10: Replace the input attribute disabled with readonly on the element
with id "display" so the field remains focusable/selectable; update any related
styling or JS that checks disabled (e.g., code referencing
document.getElementById("display").disabled) to instead handle readonly if
present and ensure ARIA/labeling still applies for accessibility.
In `@calculator-app/script.js`:
- Around line 3-10: The calculate() function currently uses eval(display.value)
which is unsafe; replace it with a safe expression evaluator that only accepts
digits, whitespace, parentheses and the four basic operators (+, -, *, /) and
rejects any other characters, then parse and compute the result (e.g., implement
a tokenizer + shunting-yard or simple recursive descent evaluator) and assign
the numeric result back to display.value; also ensure press() only allows adding
valid tokens or update calculate() to re-validate/sanitize display.value before
parsing and handle parse errors in the existing try/catch.
---
Nitpick comments:
In `@calculator-app/index.html`:
- Around line 13-33: Replace inline onclick attributes on all buttons by adding
data-action or distinct IDs and wiring them up in your JS: query-select the
buttons (e.g., buttons with data-action="digit", data-action="operator",
data-action="clear-all", data-action="clear-last", data-action="equals") and
attach event listeners that call the existing functions clearAll, clearLast,
press (pass the button value), and calculate respectively; remove the onclick
attributes from the HTML so behavior is driven by addEventListener calls and use
event.currentTarget or dataset to get the button value when invoking press.
In `@calculator-app/style.css`:
- Around line 6-15: The button layout should use an explicit CSS grid: add a
.buttons rule (targeting the container that holds the button elements inside
.calculator) and set display: grid, a fixed grid-template-columns (e.g.,
repeat(4, 1fr) for four columns) and a gap for spacing so buttons no longer rely
on inline wrapping; keep existing button size rules but remove reliance on
implicit wrapping so classes .calculator, .buttons and button are used to
enforce deterministic layout across zoom/font changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ebbbb09-4089-4aa5-bd9c-8efffb2112b0
📒 Files selected for processing (3)
calculator-app/index.htmlcalculator-app/script.jscalculator-app/style.css
| <body> | ||
|
|
||
| <div class="calculator"> | ||
| <input type="text" id="display" disabled /> |
There was a problem hiding this comment.
Prefer readonly over disabled for the display field.
disabled prevents focus/selection and is less accessible for a calculator display.
✅ Suggested HTML tweak
- <input type="text" id="display" disabled />
+ <input type="text" id="display" readonly aria-label="Calculator display" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <input type="text" id="display" disabled /> | |
| <input type="text" id="display" readonly aria-label="Calculator display" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@calculator-app/index.html` at line 10, Replace the input attribute disabled
with readonly on the element with id "display" so the field remains
focusable/selectable; update any related styling or JS that checks disabled
(e.g., code referencing document.getElementById("display").disabled) to instead
handle readonly if present and ensure ARIA/labeling still applies for
accessibility.
Comment on lines
+3
to
+10
| function press(val) { | ||
| display.value += val; | ||
| } | ||
|
|
||
| function calculate() { | ||
| try { | ||
| display.value = eval(display.value); // (ok for practice) | ||
| } catch { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there is no remaining use of eval in JS files.
rg -nP --type=js '\beval\s*\('Repository: florinpop17/app-ideas
Length of output: 153
Remove eval from expression execution.
eval on calculator input is a security blocker. Replace it with a parser/evaluator limited to digits/operators.
🔒 Safer approach (no dynamic code execution)
+function evaluateExpression(expression) {
+ const sanitized = expression.replace(/\s+/g, "");
+ const tokens = sanitized.match(/\d+(\.\d+)?|[()+\-*/]/g);
+ if (!tokens || tokens.join("") !== sanitized) throw new Error("Invalid expression");
+
+ const prec = { "+": 1, "-": 1, "*": 2, "/": 2 };
+ const output = [];
+ const ops = [];
+
+ for (const t of tokens) {
+ if (/^\d/.test(t)) output.push(Number(t));
+ else if (t in prec) {
+ while (ops.length && ops[ops.length - 1] in prec && prec[ops[ops.length - 1]] >= prec[t]) {
+ output.push(ops.pop());
+ }
+ ops.push(t);
+ } else if (t === "(") ops.push(t);
+ else if (t === ")") {
+ while (ops.length && ops[ops.length - 1] !== "(") output.push(ops.pop());
+ if (ops.pop() !== "(") throw new Error("Mismatched parentheses");
+ }
+ }
+ while (ops.length) {
+ const op = ops.pop();
+ if (op === "(") throw new Error("Mismatched parentheses");
+ output.push(op);
+ }
+
+ const stack = [];
+ for (const t of output) {
+ if (typeof t === "number") stack.push(t);
+ else {
+ const b = stack.pop();
+ const a = stack.pop();
+ if (a === undefined || b === undefined) throw new Error("Invalid expression");
+ stack.push(t === "+" ? a + b : t === "-" ? a - b : t === "*" ? a * b : a / b);
+ }
+ }
+ if (stack.length !== 1) throw new Error("Invalid expression");
+ return stack[0];
+}
+
function calculate() {
try {
- display.value = eval(display.value); // (ok for practice)
+ display.value = String(evaluateExpression(display.value));
} catch {
display.value = "ERR";
}
}🧰 Tools
🪛 Biome (2.4.10)
[error] 9-9: eval() exposes to security risks and performance issues.
(lint/security/noGlobalEval)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@calculator-app/script.js` around lines 3 - 10, The calculate() function
currently uses eval(display.value) which is unsafe; replace it with a safe
expression evaluator that only accepts digits, whitespace, parentheses and the
four basic operators (+, -, *, /) and rejects any other characters, then parse
and compute the result (e.g., implement a tokenizer + shunting-yard or simple
recursive descent evaluator) and assign the numeric result back to
display.value; also ensure press() only allows adding valid tokens or update
calculate() to re-validate/sanitize display.value before parsing and handle
parse errors in the existing try/catch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature: Calculator App
What I implemented:
Files added:
How to test:
Summary by CodeRabbit
New Features